Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add callback to NatsAuthOpts that allows refreshing a Token #712

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

garrett-sutton
Copy link
Contributor

@garrett-sutton garrett-sutton commented Jan 13, 2025

Adds the following to NatsAuthOpts

  • Func<Uri, CancellationToken, ValueTask<NatsAuthCred>>? AuthCredCallback

The purpose of this PR is to allow for use cases to refresh a Token, JWT, or NKey associated with their NatsConnection during a reconnect scenario.

resolves #356

@garrett-sutton garrett-sutton marked this pull request as ready for review January 13, 2025 23:18
@garrett-sutton

This comment was marked as resolved.

@mtmk

This comment was marked as resolved.

@garrett-sutton garrett-sutton force-pushed the auth-callback branch 4 times, most recently from 0bdb854 to fb423fb Compare January 14, 2025 23:06
@garrett-sutton

This comment was marked as resolved.

@caleblloyd
Copy link
Collaborator

The most recent options callback I reviewed did supply a URI and a CancellationToken as an argument

/// <summary>
/// An optional async callback handler for manipulation of ClientWebSocketOptions used for WebSocket connections.
/// Implementors should use the passed CancellationToken for async operations called by this handler.
/// </summary>
public Func<Uri, ClientWebSocketOptions, CancellationToken, ValueTask>? ConfigureClientWebSocketOptions { get; init; } = null;

API usability point: should there be a single callback rather than individual ones?

Sounds like a good idea, a signature could be

In NatsAuthOpts:

   Func<Uri, CancellationToken, ValueTask<NatsAuthOpts>? Callback { get; init; } = null;

This would have to come with the caveat that a Callback could not return another NatsOptsAuth with a Callback

Or in NatsOpts:

   Func<Uri, CancellationToken, ValueTask<NatsAuthOpts>? AuthOptsCallback { get; init; } = null;

Could also be a slipper slope though, what other options could be updated between reconnects? And which ones could be updated after knowing the URI that will be connected to? Is it worth putting just the auth ones in now, or coming up with a broader approach to allow for updating all potential options

@mtmk

This comment was marked as resolved.

@garrett-sutton

This comment was marked as resolved.

@mtmk

This comment was marked as resolved.

@caleblloyd

This comment was marked as resolved.

@mtmk

This comment was marked as resolved.

Copy link
Collaborator

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how the API turned out!

src/NATS.Client.Core/Internal/ClientOpts.cs Show resolved Hide resolved
@garrett-sutton
Copy link
Contributor Author

@mtmk at this point, do you have a feeling on how close we are to approval/merging?

And as a follow-up, how long can I expect until a new release is cut with these changes?

@mtmk
Copy link
Collaborator

mtmk commented Jan 23, 2025

@garrett-sutton I'd like to allow a few days for others to have a chance to review and comment. All being well, I'd say we can merge and release early next week.

@garrett-sutton
Copy link
Contributor Author

@mtmk sounds good. I'm looking to utilize the new feature in a new project at my org that we're trying to release soon. I just wanted to check in on where we are at. Thanks!

mtmk

This comment was marked as resolved.

gsutton added 2 commits January 24, 2025 09:40
Add factory methods for NatsAuthCred
Copy link
Collaborator

@mtmk mtmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @garrett-sutton

@mtmk mtmk merged commit c94fe02 into nats-io:main Jan 24, 2025
7 of 10 checks passed
mtmk added a commit that referenced this pull request Jan 27, 2025
* Fix telemetry header readonly error (#719)
* Add NatsClient to DI (#689)
* Add callback to NatsAuthOpts that allows refreshing a Token (#712)
* Implement Lame Duck Mode Event Handler (#716)
* Add CreateOrUpdateStoreAsync (#707)
@mtmk mtmk mentioned this pull request Jan 27, 2025
mtmk added a commit that referenced this pull request Jan 27, 2025
* Fix telemetry header readonly error (#719)
* Add NatsClient to DI (#689)
* Add callback to NatsAuthOpts that allows refreshing a Token (#712)
* Implement Lame Duck Mode Event Handler (#716)
* Add CreateOrUpdateStoreAsync (#707)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow updating the Connection's token and/or JWT upon disconnect
4 participants